QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Are you concerned about the operations becoming a lot bigger?

Codecov Report

Attention: Patch coverage is 91.42857% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.84%. Comparing base (834f713) to head (e10532e).

@@            Coverage Diff             @@
##             main    #5206      +/-   ##
==========================================
+ Coverage   85.83%   85.84%   +0.01%     
==========================================
  Files          94       94              
  Lines       34758    34848      +90     
==========================================
+ Hits        29833    29914      +81     
- Misses       4925     4934       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is a good addition, I think the artifactId's will help a lot for getting these other edit flows to work. One issue I see is with shell, which admittedly is probably one of the trickiest cases.

With the following KCL:

sketch001 = startSketchOn('XZ')
  |> circle({ center = [0, 0], radius = 174.66 }, %)
extrude001 = extrude(100, sketch001)
shell001 = shell({ faces = ['end'], thickness = 40 }, extrude001)

I get the following JS object for the operations:

[
    {
        "type": "StdLibCall",
        "name": "startSketchOn",
        "unlabeledArg": null,
        "labeledArgs": {
            "data": {
                "value": {
                    "type": "String",
                    "value": "XZ",
                    "__meta": [
                        {
                            "sourceRange": [
                                26,
                                30,
                                0
                            ]
                        }
                    ]
                },
                "sourceRange": [
                    26,
                    30,
                    0
                ]
            }
        },
        "sourceRange": [
            12,
            31,
            0
        ]
    },
    {
        "type": "StdLibCall",
        "name": "extrude",
        "unlabeledArg": null,
        "labeledArgs": {
            "length": {
                "value": {
                    "type": "Number",
                    "value": 100,
                    "__meta": [
                        {
                            "sourceRange": [
                                106,
                                109,
                                0
                            ]
                        }
                    ]
                },
                "sourceRange": [
                    106,
                    109,
                    0
                ]
            },
            "sketch_set": {
                "artifactIds": [
                    "487cfafc-927a-423d-9e6b-63ed3ac2dba0"
                ],
                "sourceRange": [
                    111,
                    120,
                    0
                ]
            }
        },
        "sourceRange": [
            98,
            121,
            0
        ]
    },
    {
        "type": "StdLibCall",
        "name": "shell",
        "unlabeledArg": null,
        "labeledArgs": {
            "data": {
                "sourceRange": [
                    139,
                    174,
                    0
                ]
            },
            "solid_set": {
                "artifactIds": [
                    "dac9e1e6-0852-475e-9464-f25146fb423d"
                ],
                "sourceRange": [
                    176,
                    186,
                    0
                ]
            }
        },
        "sourceRange": [
            133,
            187,
            0
        ]
    }
]

I think I can get to the proper selection of the face artifact using the artifactId for the solid_set value plus the faces value pulled using the sourceRange for the data argument, but it is still pretty intense. This is another +1 for your proposal for flatter stdlib functions. But I'm wondering if the data function could have an artifactId field like the solid_set field has? Then based on context clues I could know (at least in this case) that any artifactIds on the data argument would be the face artifacts, which are what I want.

It seems like you want the full nested structure, which I was trying to avoid, but doing that should provide all the info you could ever want. I duplicated the entire structure of KclValue, replacing the parts that the UI doesn't need with only artifactId. I call it OpKclValue.

This is what a shell operation looks like.

  {
    "labeledArgs": {
      "faces": {
        "value": {
          "type": "Array",
          "value": [
            {
              "type": "String",
              "value": "end"
            }
          ]
        },
        "sourceRange": [
          97,
          104,
          0
        ]
      },
      "thickness": {
        "value": {
          "type": "Number",
          "value": 0.25
        },
        "sourceRange": [
          118,
          122,
          0
        ]
      }
    },
    "name": "shell",
    "sourceRange": [
      83,
      123,
      0
    ],
    "type": "StdLibCall",
    "unlabeledArg": {
      "value": {
        "type": "Solid",
        "value": {
          "artifactId": "[uuid]"
        }
      },
      "sourceRange": [
        0,
        0,
        0
      ]
    }
  }

1. Should the KCL expression arguments include their raw unparsed string value?

I guess I'm a little confused. We should have a huddle about this. Because it seems contradictory to want to use the expression, but also have the nested values. Example:

fn compute() {
  return {
    custom = {
      axis = [0, 0, 1],
      origin = [0, 0.25, 0],
    }
  }
}
x = compute()
helix(axis = x, ...)

For the axis argument, the expression is x. But the value is:

{
  custom = {
    axis = [0, 0, 1],
    origin = [0, 0.25, 0],
  }
}

I thought you wanted the value so that you could access deeply into the object, like origin. But if we do that, there's no meaningful expression at the call site. It's just a variable reference x. Does that make sense?

We already have the source range for the expression. So if you want to get the KCL of the expression x, you can do that. But it doesn't really make sense to have the KCL expression for origin because it's somewhere else, inside a function in this example.

2. I don't understand the ty additional value appearing with KCL expressions, they don't seem to add any information about how to use the expression's value?

Units are part of numbers now in KCL. A lone numeric value is ambiguous without its units. The numeric type now includes what dimension and unit it is. I added it because it's available in the interpreter now, and it seems like we'd want the user to be able to see and/or edit it. I know that explicit units as in 5mm aren't used much in KCL today. But every day, support is growing. Just the other day, we had a discussion in Slack that cos(90deg) and cos(3.14rad) will do the right thing. That can only work if units are supported everywhere.

3. Should we continue to include the artifactId for scene objects if we're planning on trying to filter those arguments from the edit flows? My thought is yes but just wanted to check

It seems useful because it allows us to tie it to the artifact graph. But if you don't think you need it, I'm happy to remove as much as possible from being exposed.

You're absolutely right @jtran, thanks for huddling with me.

1. You are so right. I apologize, I lost context while doing other work, and forgot that we planned to use the source ranges to populate the command palette with KCL expression values.
2. This is very exciting and makes sense. It's interesting that these defaults are passed along everywhere when the UoM is unknown, but I understand it far better now
3. I think we should leave it in because rollback and editing of selection-based arguments is something I would like to continue work on immediately after we have confirmed our survival. It doesn't have impact on performance in the meantime